-
Notifications
You must be signed in to change notification settings - Fork 2.2k
rootfs: only set mode= for tmpfs mount if target already existed #4973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0a6577f to
8d0921c
Compare
|
This patch seems to trigger errors from criu-dev more consistently than the flakes we've been seeing in the past few weeks... Odd... |
In fact, I suggest to remove the |
|
I saw that, but the fact it fails more consistently with this change seems to imply it's not the same flake. Normally retrying the job succeeds after one (or maybe two) attempts, but this PR has failed >10 times in a row with multiple tests failing. Also note that criu-dev will eventually be released and we will have to deal with the same issue in the future, so I don't really see the benefit of just removing the CI job. @kolyshkin has spent some time debugging the issue in checkpoint-restore/criu#2781 and it appears that we found an issue in CRIU thanks to this testing. |
|
I think we already have a CI job that tests the latest stable version of CRIU, so testing the project in development mode doesn’t really make sense—at least in my opinion. I could be wrong, though! |
|
@rst0git The error I saw yesterday was different and it was CRIU related, there is a separate flake I need to work around. I'll get you a copy of the logs on Monday. |
|
|
And of course now it passes when I ran it again... |
This was always the intended behaviour but commit 72fbb34 ("rootfs: switch to fd-based handling of mountpoint targets") regressed it when adding a mechanism to create a file handle to the target if it didn't already exist (causing the later stat to always succeed). A lot of people depend on this functionality, so add some tests to make sure we don't break it in the future. Fixes: 72fbb34 ("rootfs: switch to fd-based handling of mountpoint targets") Signed-off-by: Aleksa Sarai <[email protected]>
|
This is ready to review by the way, @opencontainers/runc-maintainers. |
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The Python classic repo's CI just started failing in the container-test job with: `mkdir: cannot create directory '/app/.heroku': Permission denied` eg: https://github.com/heroku/heroku-buildpack-python/actions/runs/19368179568/job/55418539741 After updating Docker locally, I was able to reproduce the error, and have found it's due to the recent runc 1.33 release: https://github.com/opencontainers/runc/releases/tag/v1.3.3 This runc release includes a number of security fixes - however, one of which has a regression: opencontainers/runc#4971 There is a fix for this upstream: opencontainers/runc#4973 ...but it's not released yet. However, we can work around the issue by explicitly setting the previous tmpfs permissions using `:mode=1777`: https://docs.docker.com/engine/storage/tmpfs/#options-for---tmpfs GUS-W-20221627.
The Python classic repo's CI just started failing in the container-test job with: `mkdir: cannot create directory '/app/.heroku': Permission denied` eg: https://github.com/heroku/heroku-buildpack-python/actions/runs/19368179568/job/55418539741 After updating Docker locally, I was able to reproduce the error, and have found it's due to the recent runc 1.3.3 release: https://github.com/opencontainers/runc/releases/tag/v1.3.3 This runc release includes a number of security fixes - however, one of which has a regression: opencontainers/runc#4971 There is a fix for this upstream: opencontainers/runc#4973 ...but it's not released yet. However, we can work around the issue by explicitly setting the previous tmpfs permissions using `:mode=1777`: https://docs.docker.com/engine/storage/tmpfs/#options-for---tmpfs GUS-W-20221627.
This was always the intended behaviour but commit 72fbb34 ("rootfs:
switch to fd-based handling of mountpoint targets") regressed it when
adding a mechanism to create a file handle to the target if it didn't
already exist (causing the later stat to always succeed).
A lot of people depend on this functionality, so add some tests to make
sure we don't break it in the future.
Fixes #4971
Fixes: 72fbb34 ("rootfs: switch to fd-based handling of mountpoint targets")
Signed-off-by: Aleksa Sarai [email protected]